Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update LinkDestinationSummary to support multi-language content #54

Merged

Conversation

d-ronnqvist
Copy link
Contributor

Bug/issue #, if applicable: rdar://83716043

Summary

This updates LinkDestinationSummary and the LinkableEntities.json spec to support multi-language content by moving the properties that can vary into a ContentVariant substructure.

The implementation supports decoding data in the "0.1.0" format but the encoded data will always be in the "0.2.0" format.

Dependencies

As a code change this doesn't depend on #47 but the changes in this PR are not useful without the base support for multi-language documentation catalogs.

Testing

Convert a multi-language documentation catalog and include the --emit-digest flag so that Swift-DocC will create the linkable-entities.json file.

(For testing purposes it may be convenient to also set the DOCC_JSON_PRETTYPRINT environment to YES to get output that's more easily inspectable.)

The content of the linkable-entities.json file should include the variant content of each interface language for each "entity".

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary

@d-ronnqvist
Copy link
Contributor Author

Most of this is ready to review but the LinkDestinationSummary only creates a ContentVariant for the documentation node's sourceLanguage property, not for all the available source languages.

@d-ronnqvist
Copy link
Contributor Author

I opened https://bugs.swift.org/browse/SR-15574 as a follow-up change to update the implementation to write content variants for all source languages.

@d-ronnqvist d-ronnqvist marked this pull request as ready for review December 9, 2021 22:59
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

throw DecodingError.dataCorruptedError(forKey: .contentVariants, in: container, debugDescription: "Missing required content. ContentVariations is empty.")
}
self.contentVariants = contentVariants
} catch DecodingError.keyNotFound(_, let originalErrorContext) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do a version check instead (== "0.1.0") for the legacy decoding rather than falling back to it if the variants key is missing, or do you think it's not worth it?

Copy link
Contributor Author

@d-ronnqvist d-ronnqvist Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would require that we encode the version information in the JSON, which we would probably do once for the entire file instead of once for per element.

@@ -271,6 +302,71 @@ class ExternalLinkableTests: XCTestCase {
}
}

func testDecodingLegacyData() throws {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@d-ronnqvist
Copy link
Contributor Author

I made some changes to the spec and the implementation to reduce the amount of duplicate encoded data when some content is the same in multiple languages.

This also makes the single source language content almost the same as previously (the only difference is that language has been replaced with traits but the new implementation can decode the language into a trait.

Since the changes are so small we could decide to redundantly encode the language to make new content backwards compatible.

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@ethan-kusters ethan-kusters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me! I left a few documentation nits but happy to approve now.

/// The traits of the variant.
public let traits: [RenderNode.Variant.Trait]

/// A wrapper for variant values that are either set (the variant has a custom value) not set (the variant have the same value as the summarized element)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// A wrapper for variant values that are either set (the variant has a custom value) not set (the variant have the same value as the summarized element)
/// A wrapper for variant values that can either be specified, meaning the variant has a custom value, or not, meaning the variant has the same value as the summarized element.

/// This alias is used to make the property declarations more explicit while at the same time offering the convenient syntax of optionals.
public typealias VariantValue = Optional

/// The kind of the variant or `nil` if the kind is the same as the summarized element.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use .none here maybe? To differentiate between the wrapper and the inner value a little more.

Suggested change
/// The kind of the variant or `nil` if the kind is the same as the summarized element.
/// The kind of the variant or `.none` if the kind is the same as the summarized element.


/// The abstract of the variant or `nil` if the abstract is the same as the summarized element.
///
/// - Note: If the summarized element has an abstract but the variant doesn't, this property will be `Optional.some(nil)`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Note's render kind of strangely if they're not embedded in other content. It might be better to just make this a regular discussion.

Suggested change
/// - Note: If the summarized element has an abstract but the variant doesn't, this property will be `Optional.some(nil)`.
/// If the summarized element has an abstract but the variant doesn't, this property will be `Optional.some(nil)`.

/// The kind of the variant or `nil` if the kind is the same as the summarized element.
public let kind: VariantValue<DocumentationNode.Kind>

/// The language of the variant or `nil` if the kind is the same as the summarized element.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The language of the variant or `nil` if the kind is the same as the summarized element.
/// The source language of the variant or `nil` if the kind is the same as the summarized element.

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit 9808092 into swiftlang:main Dec 16, 2021
d-ronnqvist added a commit to d-ronnqvist/swift-docc that referenced this pull request Dec 20, 2021
…tlang#54)

* Move content into a source language variant structure

rdar://83716043

* Update LinkDestinationSummary to match spec changes

rdar://83716043

* Move `usr` out of variant substructure since it doesn't vary

* Update the spec to reduce the amount of duplicate data.

Now, the data continues to be encoded directly in the summary and each variant only sets the information that's different in that source language.

* Remove extra closing back-tick in documentation comments

* Use an alias to make optionality of variant properties more explicit

* Remove trait from main summary element to avoid using it as a language replacement

* Small documentation comment refinements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants